Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BE] socket 분리 로직 수정 #91

Merged
merged 5 commits into from
Nov 12, 2024
Merged

[BE] socket 분리 로직 수정 #91

merged 5 commits into from
Nov 12, 2024

Conversation

sieunie
Copy link
Collaborator

@sieunie sieunie commented Nov 12, 2024

✅ 주요 작업

  • onModuleInit이 한번만 실행되도록 로직 수정

💭 고민과 해결과정

  • BaseSocketService를 extend 하고 싶었지만... onModuleInit이 여러번 실행되어 소켓 세션이 여러개 연결되는 문제가 발생했다. 한투에서는 소켓 세션을 하나만 허용하고 있기 때문에, 연결 오류가 발생해 전체적으로 로직을 수정했다.

@sieunie sieunie added BE 백엔드 BUGFIX 버그 픽스 labels Nov 12, 2024
@sieunie sieunie requested a review from uuuo3o November 12, 2024 10:46
@sieunie sieunie self-assigned this Nov 12, 2024
Copy link
Collaborator

@uuuo3o uuuo3o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다!

@@ -22,7 +22,7 @@ import { RequestInterface } from './interface/request.interface';
@Controller('/api/stocks/trade')
@ApiTags('주식 매수/매도 API')
export class StockOrderController {
constructor(private readonly stockTradeService: StockOrderService) {}
constructor(private readonly stockOrderService: StockOrderService) {}
Copy link
Collaborator

@uuuo3o uuuo3o Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 이거 socket 분리 로직 수정 커밋 맞나요?? 소켓이랑 상관 없어보이는 코드 수정 같아 보이는데 커밋 나눠서 작성해주시면 더 좋을 거 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에고 그렇네요ㅜ 다음부터 유의해서 커밋 올리겠습니당


@Injectable()
export class StockIndexSocketService extends BaseSocketService {
export class StockIndexSocketService {
private TRADE_CODE = 'H0UPCNT0';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 제 개인적인 취향을 말하자면,, 한투에서 TR_ID 라는 이름으로 값을 받고 있으면 여기서도 동일하게 작성하는게 예뻐보이는데 어떻게 생각하시나요..?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 그게 더 좋을 것 같네요! 수정해서 커밋 추가했습니다.

(data: string[]) => {
this.socketGateway.sendStockIndexValueToClient(
this.STOCK_CODE[data[0]],
new StockIndexValueElementDto(data[2], data[4], data[9], data[3]),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 여기도 위에 코스피, 코스닥 이런 주석 단 것처럼 data[2], data[4] 이런 값이 어떤건지 주석으로 달아두면 코드 읽기가 더 좋을 거 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니당 주석 추가해두었습니다~!

@uuuo3o
Copy link
Collaborator

uuuo3o commented Nov 12, 2024

헉 수정이 짱 빠르시네요.. 감동!
하나만 더 코멘트하자면 주석 추가를 지금 chore로 올리셨는데 저희 깃 컨벤션 보시면
💡 comment 라는 친구가 있어요! 다음부터는 이거 사용해주셔도 좋을 거 같아요 ㅎㅎ

@sieunie sieunie merged commit 02230c3 into back/main Nov 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 BUGFIX 버그 픽스
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants